Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Feature/362 categories #378

Merged
merged 52 commits into from
Apr 14, 2021
Merged

Feature/362 categories #378

merged 52 commits into from
Apr 14, 2021

Conversation

JVerbruggen
Copy link
Member

@JVerbruggen JVerbruggen commented Feb 24, 2021 •
edited by niraymak
Loading

Categories

Description

The addition of tags can give users a better insight into certain characteristics of projects. This pull request introduces tags to dex.

Type of change

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • This change requires a documentation update

Checklist

  • My code follows the style guidelines of this project
  • I did not update API Controllers, if I did, I added/changed Postman tests
  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes
  • I updated the changelog with an end-user readable description
  • I assigned this pull request to the correct project board to update the sprint board

Steps to Test or Reproduce

Outline the steps to test or reproduce the PR here.
These steps will be used during release testing.

  1. Do something here...
  2. Then run this test...
  3. After that, you should see the result here

Link to issue

Closes: #362

@JVerbruggen JVerbruggen changed the title Tags Feature/362 Tags Feb 26, 2021
@JVerbruggen JVerbruggen self-assigned this Feb 26, 2021
@JVerbruggen JVerbruggen linked an issue Feb 26, 2021 that may be closed by this pull request
Research: How can we tag and categorize projects? #58
Closed
@JVerbruggen JVerbruggen marked this pull request as ready for review February 26, 2021 13:42
RubenFricke
RubenFricke requested changes Feb 27, 2021
View reviewed changes
Copy link
Member

@RubenFricke RubenFricke left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome job, congratz on your first PR this semester 🎉! Well done, I can really see you understand the underlying architecture of the application.

Your pull request did not have a description and steps to reproduce. I understand the general idea of this pull request, but it would be nice if you could update the pull request anyways (it's easier for others to review).

Your code looked really good. I found some small notes and made some suggestions, check if you agree with me (if you don't agree, let me know why 😉). Keep up the good work Jurjen!

API/Controllers/TagController.cs Outdated Show resolved Hide resolved
API/Controllers/TagController.cs Outdated Show resolved Hide resolved
API/Controllers/TagController.cs Outdated Show resolved Hide resolved
API/Controllers/TagController.cs Outdated Show resolved Hide resolved
API/Controllers/TagController.cs Outdated Show resolved Hide resolved
IdentityServer/6_IdentityServer.csproj Outdated Show resolved Hide resolved
Repositories/TagRepository.cs Outdated Show resolved Hide resolved
Services.Tests/RoleServiceTest.cs Outdated Show resolved Hide resolved
API/Controllers/TagController.cs Outdated Show resolved Hide resolved
API/Controllers/TagController.cs Outdated Show resolved Hide resolved
@JVerbruggen
Copy link
Member Author

@Ruby77

Your pull request did not have a description and steps to reproduce. I understand the general idea of this pull request, but it would be nice if you could update the pull request anyways (it's easier for others to review).

What should be considered as steps to reproduce in this case? Reproduce what? Expected functionality?

@RubenFricke
Copy link
Member

@Ruby77

Your pull request did not have a description and steps to reproduce. I understand the general idea of this pull request, but it would be nice if you could update the pull request anyways (it's easier for others to review).

What should be considered as steps to reproduce in this case? Reproduce what? Expected functionality?

Good question @JVerbruggen! In Steps to Test or Reproduce you explain how to test your code. So in your case, this could be something like this: "Execute the CRUD functionality from the tag controller as admin and validate that other roles are not able to do this. Validate you are able to add, update and delete tags on your own project. Validate that the data officer is able to update/add/delete tags on projects from users from the same institute. And finally, validate that the admin is able to update/add/delete tags on projects from everyone.".

I hope this makes sense, let me know otherwise 😉

niraymak
niraymak requested changes Mar 3, 2021
View reviewed changes
Copy link
Member

@niraymak niraymak left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nicely done! I have a few requests. Good job that you also updated Postman tests and unit tests. 🥇

We will meet about the system tomorrow to discuss if the implementation matches how we envision it.

API/Controllers/ProjectController.cs Outdated Show resolved Hide resolved
API/Controllers/ProjectController.cs Outdated Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
IdentityServer/6_IdentityServer.csproj Outdated Show resolved Hide resolved
Data/Helpers/Seed.cs Outdated Show resolved Hide resolved
Services/Services/ProjectTagService.cs Outdated Show resolved Hide resolved
Repositories/TagRepository.cs Outdated Show resolved Hide resolved
Repositories/ProjectTagRepository.cs Outdated Show resolved Hide resolved
Services.Tests/TagServiceTest.cs Outdated Show resolved Hide resolved
Repositories/TagRepository.cs Outdated Show resolved Hide resolved
@JVerbruggen
Copy link
Member Author

  • These tags are going to be renamed to categories

TimSnoek123
TimSnoek123 previously approved these changes Apr 8, 2021
View reviewed changes
niraymak
niraymak requested changes Apr 8, 2021
View reviewed changes
Copy link
Member

@niraymak niraymak left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good work! 👍

I tried to update the categories of an existing project but something went wrong mapping the resource to an actual project. Maybe just to be sure you can also add a check for this in the integration tests.

Also found an issue for the authorization check.

Maybe since you have to make some changes you can also fix Tim's feedback.

API/Controllers/ProjectController.cs Show resolved Hide resolved
Models/Project.cs Outdated Show resolved Hide resolved
Data/Migrations/ApplicationDbContextModelSnapshot.cs Outdated Show resolved Hide resolved
API/Controllers/ProjectController.cs Show resolved Hide resolved
niraymak
niraymak requested changes Apr 12, 2021
View reviewed changes
Copy link
Member

@niraymak niraymak left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Results for request for projectId 59:
image

Tried to update categories:
image

Project categories not updated succesfully.
image

niraymak
niraymak previously approved these changes Apr 12, 2021
View reviewed changes
Copy link
Member

@niraymak niraymak left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for your implementation :) Good work! 💯

niraymak
niraymak previously approved these changes Apr 12, 2021
View reviewed changes
RubenFricke
RubenFricke approved these changes Apr 14, 2021
View reviewed changes
Copy link
Member

@RubenFricke RubenFricke left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Everything works flawless. Thanks for the pull request!

@JVerbruggen JVerbruggen merged commit 596281a into develop Apr 14, 2021
@JVerbruggen JVerbruggen deleted the feature/362-add-tags-categories branch April 14, 2021 13:24
TimSnoek123
TimSnoek123 approved these changes Apr 14, 2021
View reviewed changes
Copy link
Contributor

@TimSnoek123 TimSnoek123 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good 👍

@niraymak niraymak changed the title Feature/362 Tags Feature/362 categories Apr 15, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@BluRRayS BluRRayS BluRRayS left review comments

@niraymak niraymak niraymak approved these changes

@TimSnoek123 TimSnoek123 TimSnoek123 approved these changes

@RubenFricke RubenFricke RubenFricke approved these changes

Assignees

@JVerbruggen JVerbruggen

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Add categories Research: How can we tag and categorize projects?
5 participants
@JVerbruggen @RubenFricke @niraymak @TimSnoek123 @BluRRayS